Skip to content

fix: remove dead code in MerkleDAG.compare() — modified detection is unreachable#294

Open
Poojan-patel wants to merge 1 commit intozilliztech:masterfrom
Poojan-patel:fix/remove-dead-code-merkle-compare
Open

fix: remove dead code in MerkleDAG.compare() — modified detection is unreachable#294
Poojan-patel wants to merge 1 commit intozilliztech:masterfrom
Poojan-patel:fix/remove-dead-code-merkle-compare

Conversation

@Poojan-patel
Copy link
Copy Markdown

Problem

MerkleDAG.compare() contains a modified detection loop that can never produce results:

const modified: string[] = [];
for (const [id, node1] of Array.from(nodes1.entries())) {
    const node2 = nodes2.get(id);
    if (node2 && node1.data !== node2.data) {
        modified.push(id);
    }
}
Node IDs are computed as SHA-256(data). Since the ID is the hash of the data, it is cryptographically impossible for two nodes to share the same ID but have different data. The condition node2 && node1.data !== node2.data can never be true.

When a file is modified, its content hash changes, which changes its node data ("path:hash"), which changes its node ID. The old node ID appears in removed and the new one appears in added  but modified is always [].

Why the system still works correctly
MerkleDAG.compare() is only used as a fast "did anything change?" boolean gate inside FileSynchronizer.checkForChanges(). The actual categorization of files into added/removed/modified is done by compareStates(), which compares Map<filePath, contentHash>  keyed by file path, not content hash  so it correctly identifies modifications.

MerkleDAG.compare()       "something changed" (boolean gate)
    
compareStates()           "here's exactly what changed" (added/removed/modified)
Changes
merkle.ts: Removed the unreachable modified loop from compare(). Simplified the return type from { added, removed, modified } to { added, removed }. Replaced Map with Set since only node IDs are needed for the comparison.
synchronizer.ts: Removed changes.modified check from the DAG comparison condition to match the updated return type.
Verification
pnpm build:core  passes
pnpm build:mcp  passes
No behavioral change: file modification detection continues to work via compareStates() in FileSynchronizer

The `modified` array in `MerkleDAG.compare()` was dead code. Since
node IDs are derived from SHA-256 hashing of node data, it is
cryptographically impossible for two nodes to share the same ID but
have different data. Modified files already surface correctly as
entries in both `added` and `removed`, and the actual file-level
modification detection is handled by `FileSynchronizer.compareStates()`
which keys on file paths rather than content hashes.

- Remove unreachable modified loop from `MerkleDAG.compare()`
- Simplify return type to `{ added, removed }`
- Use Set instead of Map since only node IDs are needed
- Update `checkForChanges()` condition to match new signature

Made-with: Cursor
@Poojan-patel
Copy link
Copy Markdown
Author

@zc277584121 , Can you review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant